Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proposal for adding Keyring CMM and updating Default CMM definition #220

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lavaleri
Copy link
Contributor

Issue #, if available: Proposal for reframing the Default CMM

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lavaleri lavaleri requested a review from seebees July 29, 2020 02:11
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved

The specific CMM configuration describes a safe default that serves most use cases.

The CMM configuration defined by the Default CMM is the [Keyring CMM](#keyring-cmm) as is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the Keyring CMM take an option for the default algorithm
and then the Default CMM defines this option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. If you are suggesting that we be able to initialize the KeyringCMM with a "default alg" option that overrides the default defined in the alg suite, then that would be changing the current behavior of the Default CMM and is out of scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting that the Keyring CMM takes an option for "default alg",
and then when the Keyring CMM is created in the Default CMM,
then the option would be what we define as a default.

So the ESDK default algorithm is defined in the Default CMM and not in the Keyring CMM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the keyring CMM just requires that the request identify the algorithm suite?

I'm loathe to squeeze opinions about algorithm suite into a CMM that SHOULD only care about wrangling keyrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you're proposing now. This would mean that the Default CMM is no longer just a configuration. Unless we also want to move to the "default to the default alg CMM" logic out of the Keyring CMM and into a new CMM.

If we were to do something like this, I agree with Matt's approach, the KeyringCMM should just fail if it doesn't get an alg suite. I'm not sure if splitting up the logic this way is useful or not though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but the keyring CMM should be easy to use on its own.
If customers chose to use only the keyring CMM and inject that into the ESDK, it would not work in the default case.
Maybe that is a good thing though.

What I would like is that the default algorithm suite for the ESDK is defined in the Default CMM, not the keyring CMM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I would like is that the default algorithm suite for the ESDK is defined in the Default CMM, not the keyring CMM.

Agreed. If you're opting out of the default then you don't get any of the defaults. I think this is reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, we do not want to have the Keyring CMM take a REQUIRED option "default alg"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding this conversation, but near the end of the doc, we say "This CMM MUST NOT offer any additional features beyond the composed CMM created above." To me, this means the default CMM MAY offer the same configuration options as the thing it is configuring. However, it MAY additionally support sane defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After offline discussion:

  • It is important that the Default CMM MUST represent an expression of default values without accompanying logic
    • This defines all defaults for encryption and decryption in one place, and additionally makes these defaults more extendable for any future code that may use CMMs but is not necessarily our current Encrypt/Decrypt client APIs.
    • By ensuring this is a strict configuration, we ensure that each behavior is encapsulated in a composable CMM which can be reused by customers, making it easy to compose CMMs which opt out of certain default behaviors or reuse default behaviors within another configuration.
    • This also makes updating the default behaviors easy, and able to be done in a way to preserves composability of our behaviors.
  • As such, the default algorithm suite is defined by the Default CMM
    • Customers MUST be able to get this default algorithm suite statically
  • One more behavior of the KeyringCMM in this version of the spec is that it defaults to the default algorithm. This is not desired and this behavior should be encapsulated outside the Keyring CMM implementation.
    • The Keyring CMM MUST require an algorithm suite
  • We should add a new CMM implementation DefaultRequestValues (name TBD) that is solely responsible for setting default values in the CMM's materials requests, and then passes that request to an underlying CMM.
    • For this proposal, this should ONLY be setting some default algorithm suite for the get encryption materials request.
  • The Default CMM should thus represent the composition of this new DefaultRequestValues CMM and a Keyring CMM.
  • We need to add examples for composing CMMs together.

I will update the doc with this motivation and behavior.

proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
acioc
acioc previously approved these changes Jul 31, 2020
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved

The specific CMM configuration describes a safe default that serves most use cases.

The CMM configuration defined by the Default CMM is the [Keyring CMM](#keyring-cmm) as is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding this conversation, but near the end of the doc, we say "This CMM MUST NOT offer any additional features beyond the composed CMM created above." To me, this means the default CMM MAY offer the same configuration options as the thing it is configuring. However, it MAY additionally support sane defaults.

@lavaleri
Copy link
Contributor Author

lavaleri commented Aug 7, 2020

I don't like the name "Set Unspecified Request Values CMM", but I could not think of any other name that didn't use "Default" in some way that could be interpreted wrongly. Still trying to brainstorm, but accepting recommendations!

Copy link
Contributor

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a very quick top-level question. Will read more thoroughly assuming my suggestion isn't valid :)

proposals/2020-07-28_keyring-cmm/proposal.md Outdated Show resolved Hide resolved
Any desired CMM implementation that intends to use keyrings to ensure valid materials SHOULD
compose with the Keyring CMM.

### Set Unspecified Request Values CMM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have the Encrypt operation fill in the default when not provided as input, instead of it being the CMM's responsibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have considered this (and this is what Java/Python originally did, I believe), but we want this to be defined in the CMM for two reasons:

  • The logic for everything the ESDK expresses as a "default" is encapsulated at the same level, in the Default CMM that we provide to the user. This is simple, and means that we have only one place in code where we define what "default" is.
  • If this logic is in the CMM as opposed to the client level, then any other code that theoretically can be built on top of CMMs is able to grab the defaults, instead of needing to duplicate the "set some default for the algorithm suite" behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On thinking about this further, I'm not really convinced by either reason:

  • "in the Encrypt operation" would be an equally valid single place to encapsulate the defaults
  • Setting a default value doesn't seem to be difficult enough to worry about never duplicating IF there is significant code out there using CMMs outside the context of Encrypt/Decrypt

I agree the "Set Unspecified Request Values CMM" is valid and possible, but it feels like a really heavyweight solution. It would be a lot simpler to say that CMMs require the algorithm suite to be set in all cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After offline discussion I'm at least convinced this is a reasonable solution - I'll make suggestions in the text for how we can justify it a bit more strongly.

@robin-aws
Copy link
Contributor

I don't like the name "Set Unspecified Request Values CMM", but I could not think of any other name that didn't use "Default" in some way that could be interpreted wrongly. Still trying to brainstorm, but accepting recommendations!

How about "Default-Providing CMM"?

Comment on lines +149 to +151
Customers SHOULD either use the [Default CMM](#default-cmm)
or compose a CMM using the AWS Encryption SDK's provided CMM implementations
(including the Keyring CMM).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that customers SHOULD NOT implement their own CMM, which isn't what we want. Perhaps we should say that most customers should use one of these two options, but implementing their own is also possible, and if they do they will probably want to compose it with other provided CMM implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants